Conversation
Codecov Report
@@ Coverage Diff @@
## master #461 +/- ##
==========================================
+ Coverage 86.67% 86.80% +0.12%
==========================================
Files 107 107
Lines 4219 4250 +31
==========================================
+ Hits 3657 3689 +32
+ Misses 562 561 -1
Continue to review full report at Codecov.
|
| fn gas_price<T: EnvTypes>(&mut self) -> Result<T::Balance> { | ||
| fn gas_price<T: EnvTypes>(&mut self, _gas: u64) -> Result<T::Balance> { | ||
| self.chain_spec | ||
| .gas_price::<T>() |
There was a problem hiding this comment.
It would be cool if there was an actual conversion config for the off-chain environment so that the returned amount at least scales linearly to the input.
| + TryFrom<u32> | ||
| + TryFrom<u64> | ||
| + TryFrom<u128> | ||
| + TryFrom<usize> | ||
| + TryInto<u16> | ||
| + TryInto<u32> | ||
| + TryInto<u64> | ||
| + TryInto<u128> | ||
| + TryInto<usize> | ||
| // Further trait bounds from the original BaseArithmetic trait |
There was a problem hiding this comment.
Can you explain why we need this for this PR?
There was a problem hiding this comment.
Isn't this diverging from what Substrate bounds its types?
There was a problem hiding this comment.
Can you explain why we need this for this PR?
The TryFrom<u64> is needed to convert the gas: u64 arg into EnvTypes::Balance.
Isn't this diverging from what Substrate bounds its types?
It's converging, I thought I would add the other Try* bounds while I was there: https://github.com/paritytech/substrate/blob/4be954a8463e6f33e0ec854c4b8bcd940af260fc/primitives/arithmetic/src/traits.rs#L44
| + From<u32> | ||
| + From<u8> |
There was a problem hiding this comment.
Can you explain why this change is required for this PR?
There was a problem hiding this comment.
Yes, this brings it into line with the substrate BaseArithmetic. The From<u32> bound is now defined in the AtLeast32Bit trait, and it is assumed for BaseArtithmetic trait that we are least dealing with u8
There was a problem hiding this comment.
Can you explain why this change is required for this PR?
It's not strictly required for this PR, it's just something I overlooked in #463.
There was a problem hiding this comment.
Ah okay so am I right that the above changes actually are aligning ink! further with Substrate traits? Just want to be sure about this because I really am not familiar with the Substrate tests but getting traits righti is really really important.
There was a problem hiding this comment.
Yes exactly.
We are still missing a few more e.g. the rest of the Checked* traits and UniqueSaturatedFrom/ UniqueSaturatedInto, but these are definitely out of scope for this PR.
impl<T: Sized, S: TryFrom<T> + Bounded + Sized> UniqueSaturatedFrom<T> for S {
fn unique_saturated_from(t: T) -> Self {
S::try_from(t).unwrap_or_else(|_| Bounded::max_value())
}
}
impl<T: Bounded + Sized, S: TryInto<T> + Sized> UniqueSaturatedInto<T> for S {
fn unique_saturated_into(self) -> T {
self.try_into().unwrap_or_else(|_| Bounded::max_value())
}
}
| fn gas_price<T: EnvTypes>(&mut self) -> Result<T::Balance> { | ||
| self.chain_spec | ||
| /// Emulates gas price calculation | ||
| fn gas_price<T: EnvTypes>(&mut self, gas: u64) -> Result<T::Balance> { |
There was a problem hiding this comment.
Would be nice to have a test for this function somewhere, since the logic now got more complex and logic surrounding cost analysis should be considered criticial imho. Besides this the code looks sound to me.
There was a problem hiding this comment.
Is it worth adding tests for the offchain environment?
There was a problem hiding this comment.
Theoretically yes they are worth. However, the off-chain environment currently lacks tests all over the place because originally it was not intended to stay for long since at the time of writing some standalone Seal implementation was still deemed to be realistic. Consider it a hack that came to stay.
4 participants
Rel paritytech/substrate#6478.
Adds a gas arg to
ext_gas_pricefor compatibility with the above PR.Note: the offchain impl attempts to emulate the runtime calculation by simply multiplying the provided gas amount by the gas price configure in the offchain
chain_spec.